Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

preserve sort and filter when loading more experiments #3313

Merged
merged 3 commits into from
Oct 15, 2024
Merged

preserve sort and filter when loading more experiments #3313

merged 3 commits into from
Oct 15, 2024
+57 −54

Conversation

Gkrumbach07
Copy link
Member

https://issues.redhat.com/browse/RHOAIENG-12843

Description

Fix experiment selector error when loading more results with active filters

This PR addresses an issue where clicking "View more" in the experiment selector would throw an error if sort or search filters were applied. The fix ensures that pagination works correctly while preserving the current filter state.

How Has This Been Tested?

  1. Create more than 10 experiments in the system.
  2. Navigate to the create run page.
  3. Open the experiment selector.
  4. Apply a sort filter to the experiments.
  5. Click the "View more" button.
  6. Verify that additional experiments load without errors.
  7. Apply a search filter to the experiments.
  8. Click the "View more" button again.
  9. Confirm that more experiments load correctly, respecting both sort and search filters.

Test Impact

covered by existing tests

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@codecov Codecov
Copy link

codecov bot commented Oct 8, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.81%. Comparing base (82d4f12) to head (c18eb50).
Report is 24 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3313      +/-   ##
==========================================
+ Coverage   84.77%   84.81%   +0.04%     
==========================================
  Files        1308     1315       +7     
  Lines       29251    29517     +266     
  Branches     7937     8060     +123     
==========================================
+ Hits        24798    25036     +238     
- Misses       4453     4481      +28
Files with missing lines Coverage Δ
frontend/src/api/pipelines/custom.ts 97.87% <100.00%> (+0.07%) ⬆️
.../src/concepts/pipelines/apiHooks/useExperiments.ts 100.00% <100.00%> (ø)
...ipelines/content/experiment/ExperimentSelector.tsx 96.96% <ø> (+5.30%) ⬆️
...nes/content/pipelineSelector/useCreateSelectors.ts 100.00% <100.00%> (ø)
...ts/pipelines/content/tables/usePipelineLoadMore.ts 67.64% <100.00%> (ø)
.../concepts/pipelines/context/usePipelineAPIState.ts 100.00% <ø> (ø)

... and 126 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82d4f12...c18eb50. Read the comment docs.

@pnaik1
Copy link
Contributor

pnaik1 commented Oct 11, 2024 •
edited
Loading

@Gkrumbach07 applying sort filter to the experiment works fine but I still get an error when search filter is applied

Screenshot 2024-10-11 at 3 34 41 PM

@Gkrumbach07 Gkrumbach07 force-pushed the bug/RHOAIENG-12843-experiment-selector-throwing-error-when-clicking-o branch from f1ac39d to c18eb50 Compare October 11, 2024 19:36
@Gkrumbach07
Copy link
Member Author

@pnaik1 should be fixed

jpuzz0
jpuzz0 approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@jpuzz0 jpuzz0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, works well.

@openshift-ci openshift-ci bot added the lgtm label Oct 14, 2024
@openshift-ci openshift-ci bot removed the lgtm label Oct 15, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 15, 2024

New changes are detected. LGTM label has been removed.

@jpuzz0 jpuzz0 added the lgtm label Oct 15, 2024
@Gkrumbach07
Copy link
Member Author

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gkrumbach07, jpuzz0

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit f6ec5a6 into opendatahub-io:main Oct 15, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpuzz0 jpuzz0 jpuzz0 approved these changes

@lucferbux lucferbux Awaiting requested review from lucferbux

@pnaik1 pnaik1 Awaiting requested review from pnaik1

Assignees

@jpuzz0 jpuzz0

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Gkrumbach07 @pnaik1 @jpuzz0